Nanobind migration POC#5084
Conversation
695dc8b to
576cb19
Compare
|
@soswow This is looking good! The only real concern I have at this stage is that rather than the logic for running the nanobind tests living in runtest.py, I would much rather that it all be in testing.cmake. You can see (circa testing.cmake:197) how for each of the texture tests, it actually adds them to the list a second time with different arguments to make the same test run again with in batch texturing mode (and modifying the name to append ".batch" to make it look like a separate test). I think this is how it should work for pybind11 vs nanobind as well. There should be minimal logic changes necessary in runtest.py. |
|
@soswow Ping. I proposed some changes to the testing strategy, but otherwise would like to merge what you have as a great first step to the nanobind transition. |
11448cc to
583aea9
Compare
| return std::string(spec.serialize(fmt, verb)); | ||
| }, | ||
| "format"_a = "text", "verbose"_a = "detailed") | ||
| .def( |
There was a problem hiding this comment.
The old pybind11 has a few other functions right before this and right after serialize. Seems like the following 4 were somehow missed?
.def("to_xml",
[](const ImageSpec& spec) { return PY_STR(spec.to_xml()); })
.def("from_xml", &ImageSpec::from_xml)
.def(
"valid_tile_range",
[](ImageSpec& self, int xbegin, int xend, int ybegin, int yend,
int zbegin, int zend) {
return self.valid_tile_range(xbegin, xend, ybegin, yend, zbegin,
zend);
},
"xbegin"_a, "xend"_a, "ybegin"_a, "yend"_a, "zbegin"_a = 0,
"zend"_a = 1)
.def("copy_dimensions", &ImageSpec::copy_dimensions, "other"_a)
There was a problem hiding this comment.
yeah. fair. I am pretty sure it was minimal impl. to satisfy existing tests to pass.
I will add those in (And couple more things that got left behind) and see if I Can have regression tests that would have failed if these things are not added.
There was a problem hiding this comment.
Thanks for adding the test coverage! The files looked very complete so it seemed like the 4 were accidentally forgotten and it would have been more difficult to notice later on I think. I think, for future nanobind changes that might come in more than 1 piece, any APIs remaining should be clearly marked as a TODO so it's more obvious we can't quite ship it yet.
... Hmm, just thinking out loud: I wonder if we could add coverage for this actually. We could dump the API surface with another test that does something similar to:
import pprint
pprint.pprint(dir(OpenImageIO.ImageSpec))
pprint.pprint(dir(OpenImageIO.ImageBuf))And that way we would have a good comparison against the new nanobind API surface and it would clearly show the differences. Unfortunately that would mean having to completely implement each class in a single PR...
There was a problem hiding this comment.
how about using existing stub at src/python/stubs/OpenImageIO/__init__.pyi for that?
|
@jessey-git thx for taking a look. See last 2 commits. |
jessey-git
left a comment
There was a problem hiding this comment.
I'm approving from my side though do fixup that clang-format issue from the last commit.
|
I wonder if we want those tests to run with nanobind backend in CI? It only happens when |
| * For the experimental nanobind migration backend: | ||
| * NumPy (tested through 2.2.4) | ||
| * For the nanobind (WIP) migration backend used in source/CMake builds: | ||
| * nanobind discoverable by CMake, or installed in the active Python | ||
| environment so `python -m nanobind --cmake_dir` works |
There was a problem hiding this comment.
Is this a little messed up? You have two "for the nanobind..." headers, and numpy ends up listed twice.
There was a problem hiding this comment.
yeah. will fix that now
|
These all look fine to me. I made a minor comment or two that you can address in the next round if you wish. This is more or less what I pictured the nanobind bindings looking like. Let me know if you are ready for me to merge what you have so far, and then we'll do the subsequent rounds on top of it. Again, does not have to be in this PR, but if not then soon: I think you should check in some kind of design checklist (could be in docs/dev? or as comments at the top of one of the important headers like py_oiio.h?) that enumerates what's done so far and what's left to do, that we can be checking off and adding to with subsequent PRs so it's easy to tell from a glance what is the current status as well as what jobs might be done in parallel by different people, in case anybody is in a position to help. |
|
Naive question -- Do any of our CI job variants test the nanobind path? |
yeah, as I mentioned in above message - I don't think so. I feel like it is rather important |
which is it? I think I've addressed those made on march 19 |
4efafe0 to
47ec96d
Compare
| export CC=${CC:=gcc} | ||
| export CXX=${CXX:=g++} | ||
| export OpenImageIO_CI=1 | ||
| export OpenImageIO_CI=true |
There was a problem hiding this comment.
not sure what happened here. Checking.
ba2420f to
0c6daad
Compare
Check out |
| // fill data with random values so we can hash it a bunch of different ways | ||
| std::mt19937 rng(42); | ||
| data.resize(round_to_multiple(iterations, 4) / sizeof(data[0]), 0); | ||
| data.resize(iterations / sizeof(data[0]), 0); |
There was a problem hiding this comment.
prob. missed merge mistake. investigating.
There was a problem hiding this comment.
yeap. reverted that too.
| # The optional ENVIRONMENT is a list of environment variables to set for the | ||
| # test. | ||
| # | ||
| # The optional ENVIRONMENT_MODIFICATION is a list of environment variable |
There was a problem hiding this comment.
This feature apperently requires CMake higher then 3.18.2 which is one in this project.
There was a problem hiding this comment.
I've made some changes not to have dependency on this feature so it will work on 3.18.2
…I, TypeDesc) Build as optional oiio_python_nanobind target; share tests with the pybind11 module. Documents migration status in src/python-nanobind/MIGRATION_STATUS.md. Assisted-by: Cursor/Composer-2 Signed-off-by: Aleksandr Motsjonov <soswow@gmail.com> Made-with: Cursor
Signed-off-by: Aleksandr Motsjonov <soswow@gmail.com> Made-with: Cursor
Signed-off-by: Aleksandr Motsjonov <soswow@gmail.com>
…ther table Signed-off-by: Aleksandr Motsjonov <soswow@gmail.com>
Signed-off-by: Aleksandr Motsjonov <soswow@gmail.com>
Signed-off-by: Aleksandr Motsjonov <soswow@gmail.com>
Signed-off-by: Aleksandr Motsjonov <soswow@gmail.com>
Signed-off-by: Aleksandr Motsjonov <soswow@gmail.com>
84d64c0 to
b2ef3a2
Compare
|
@jessey-git Unfortunately I've found some more divergent / missed / gaps. Feel free to see latest commits. I've squished (I believe before I Started making new changes) and other commits are all new stuff. |
Draft PR where I am exploring migration from pybind11 to nanobind.
New env flag introduced
OIIO_PYTHON_BINDINGS_BACKEND=pybind11ornanobindorbothWhen it is nanobine one is build it is in
PyOpenImageIONanobindExperimentaltarget.Checklist:
behavior.
testsuite.
PR, by pushing the changes to my fork and seeing that the automated CI
passed there. (Exceptions: If most tests pass and you can't figure out why
the remaining ones fail, it's ok to submit the PR and ask for help. Or if
any failures seem entirely unrelated to your change; sometimes things break
on the GitHub runners.)
fixed any problems reported by the clang-format CI test.
corresponding Python bindings. If altering ImageBufAlgo functions, I also
exposed the new functionality as oiiotool options.
This code contribution was assisted by Cursor/Composer-2